Reducing Warnings - Safe Integral Comparisons by eramongodb · Pull Request #960 · mongodb/mongo-c-driver

eramongodb · 2022-03-24T21:29:30Z

This is part of a series of PRs intended to reduce the set of warnings currently being observed when building the C driver.

This PR introduces a set of utilities modeled after the Safe Integral Comparisons proposal for the C++ standard library, which was accepted and merged into the C++20 standard. These utilities are extremely useful in addressing warnings involving comparisons or conversions between integers with different signedness and sizes.

The interface proposed in the original paper relies on type deduction to select appropriate behavior according to the signedness and size of the given arguments:

A a = ...
B b = ...
// no need for any cast since std::cmp_less is taking care of everything
if(std::cmp_less(a,b)){
  // do X
} else {
  // do Y
}

As C does not have type deduction required to support this interface, two major compromises are made instead:

All functions must explicitly specify the signedness of the provided argument(s).
All arguments are promoted to either int64_t or uint64_t as specified by (1) before further analysis.

Therefore, the equivalent code to the above example given A = int16_t and B = uint32_t looks like the following:

int16_t a = ...
uint32_t b = ...

if (bson_cmp_less_su (a, b)) {
  /* ... */
} else {
  /* ... */
}

where the _su suffix indicates a is signed and b is unsigned.

The set of supported types is also limited to the following:

signed and unsigned char, short, int, long, and long long (but not plain char).
signed and unsigned fixed width integers of size 8, 16, 32, and 64.
ssize_t and size_t.

More types may be added if needed, so long as for the given type T, sizeof (T) <= sizeof (uint64_t) is true and its representable range is known and defined with corresponding *_MIN and *_MAX macros.

To support the std::in_range set of functions even on C90 (where the necessary <limits.h> and <stdint.h> macros may not be defined), the corresponding set of numeric limit macros have been added to bson-compat.h, including those for ssize_t (note: SSIZE_MIN is normally not defined even by the POSIX standard). The preprocessor block conditioned on !defined(__STDC_VERSION__) || __STDC_VERSION__ < 199901L may be removed once the minimum C standard for the C driver is bumped to C99 or newer.

rcsanchez97

I have made some suggested changes based on what appear to be missing unsigned specifiers. Though, I may be mistaken so I am going to go ahead and approve the PR and leave it up to you to decide whether my suggestions are appropriate.

rcsanchez97 · 2022-03-24T22:58:21Z

src/libbson/tests/test-bson-cmp.c

+   BSON_ASSERT (!bson_cmp_less_ss (1, -1));
+   BSON_ASSERT (!bson_cmp_less_ss (1, 1));
+
+   BSON_ASSERT (!bson_cmp_less_uu (0u, 0));


Suggested change

BSON_ASSERT (!bson_cmp_less_uu (0u, 0));

BSON_ASSERT (!bson_cmp_less_uu (0u, 0u));

rcsanchez97 · 2022-03-24T22:59:03Z

src/libbson/tests/test-bson-cmp.c

+   BSON_ASSERT (bson_cmp_greater_ss (1, -1));
+   BSON_ASSERT (!bson_cmp_greater_ss (1, 1));
+
+   BSON_ASSERT (!bson_cmp_greater_uu (0u, 0));


Suggested change

BSON_ASSERT (!bson_cmp_greater_uu (0u, 0));

BSON_ASSERT (!bson_cmp_greater_uu (0u, 0u));

rcsanchez97 · 2022-03-24T22:59:30Z

src/libbson/tests/test-bson-cmp.c

+   BSON_ASSERT (!bson_cmp_less_equal_ss (1, -1));
+   BSON_ASSERT (bson_cmp_less_equal_ss (1, 1));
+
+   BSON_ASSERT (bson_cmp_less_equal_uu (0u, 0));


Suggested change

BSON_ASSERT (bson_cmp_less_equal_uu (0u, 0));

BSON_ASSERT (bson_cmp_less_equal_uu (0u, 0u));

rcsanchez97 · 2022-03-24T22:59:51Z

src/libbson/tests/test-bson-cmp.c

+   BSON_ASSERT (bson_cmp_greater_equal_ss (1, -1));
+   BSON_ASSERT (bson_cmp_greater_equal_ss (1, 1));
+
+   BSON_ASSERT (bson_cmp_greater_equal_uu (0u, 0));


Suggested change

BSON_ASSERT (bson_cmp_greater_equal_uu (0u, 0));

BSON_ASSERT (bson_cmp_greater_equal_uu (0u, 0u));

rcsanchez97 · 2022-03-24T23:00:45Z

src/libbson/tests/test-bson-cmp.c

+
+   BSON_ASSERT (bson_in_range_unsigned (int32_t, 0u));
+   BSON_ASSERT (bson_in_range_unsigned (int32_t, (uint64_t) int32_max));
+   BSON_ASSERT (!bson_in_range_unsigned (int32_t, (uint64_t) (int32_max + 1)));


Suggested change

BSON_ASSERT (!bson_in_range_unsigned (int32_t, (uint64_t) (int32_max + 1)));

BSON_ASSERT (!bson_in_range_unsigned (int32_t, (uint64_t) (int32_max + 1u)));

int32_max is an int64_t (signed), so a signed integer literal is appropriate here.

eramongodb

Passing 0 to unsigned parameter is benign since 0 is guaranteed to be representable regardless of signedness, but for consistency, I agree it would be good to update the literals with unsigned suffixes. (Hurray for copy-paste errors...)

eramongodb · 2022-03-25T15:44:40Z

src/libbson/tests/test-bson-cmp.c

+
+   BSON_ASSERT (bson_in_range_unsigned (int32_t, 0u));
+   BSON_ASSERT (bson_in_range_unsigned (int32_t, (uint64_t) int32_max));
+   BSON_ASSERT (!bson_in_range_unsigned (int32_t, (uint64_t) (int32_max + 1)));


int32_max is an int64_t (signed), so a signed integer literal is appropriate here.

kevinAlbs

LGTM. Nicely done. I think bson-cmp.h is going to be useful. I have two comments, but nothing to block merge.

kevinAlbs · 2022-04-07T16:48:16Z

src/libbson/src/bson/bson-cmp.h

+      return bson_cmp_greater_equal_uu (value, 0u) &&                       \
+             bson_cmp_less_equal_uu (value, max);                           \


Suggested change

return bson_cmp_greater_equal_uu (value, 0u) && \

bson_cmp_less_equal_uu (value, max); \

return bson_cmp_less_equal_uu (value, max); \

kevinAlbs · 2022-04-07T17:16:53Z

src/libbson/tests/test-bson-cmp.c

+   BSON_ASSERT (!bson_in_range_signed (int8_t, int8_max + 1));
+
+   BSON_ASSERT (bson_in_range_unsigned (int8_t, 0u));
+   BSON_ASSERT (bson_in_range_unsigned (int8_t, (uint64_t) int8_max));


Suggest removing unnecessary cast to uint64_t for consistency with other calls. Here and below.

These casts are necesary, as the intN_min and intN_max variables are signed integers. Without the casts, the arguments will produce implicit sign conversion warnings from int64_t to uint64_t.

eramongodb added 2 commits March 24, 2022 15:38

Add numeric limits compatibility support to bson-compat.h

7d32b8e

Add safe integral comparison functions

1c007a3

eramongodb requested review from kevinAlbs and rcsanchez97 March 24, 2022 21:29

eramongodb self-assigned this Mar 24, 2022

rcsanchez97 approved these changes Mar 24, 2022

View reviewed changes

Update literals with consistent unsigned-suffixes

6740d14

eramongodb commented Mar 25, 2022

View reviewed changes

kevinAlbs approved these changes Apr 7, 2022

View reviewed changes

Remove redundant comparison for unsigned-unsigned range check

7a7ae24

eramongodb merged commit 3032eeb into mongodb:master Apr 7, 2022

eramongodb deleted the cdriver-warnings branch April 7, 2022 18:33

eramongodb mentioned this pull request Mar 24, 2023

CDRIVER-4596 Reducing Warnings - MSVC and MinGW Warnings in libbson #1221

Merged

	BSON_ASSERT (!bson_cmp_less_uu (0u, 0));
	BSON_ASSERT (!bson_cmp_less_uu (0u, 0u));

	BSON_ASSERT (!bson_cmp_greater_uu (0u, 0));
	BSON_ASSERT (!bson_cmp_greater_uu (0u, 0u));

	BSON_ASSERT (bson_cmp_less_equal_uu (0u, 0));
	BSON_ASSERT (bson_cmp_less_equal_uu (0u, 0u));

	BSON_ASSERT (bson_cmp_greater_equal_uu (0u, 0));
	BSON_ASSERT (bson_cmp_greater_equal_uu (0u, 0u));

	BSON_ASSERT (!bson_in_range_unsigned (int32_t, (uint64_t) (int32_max + 1)));
	BSON_ASSERT (!bson_in_range_unsigned (int32_t, (uint64_t) (int32_max + 1u)));

		return bson_cmp_greater_equal_uu (value, 0u) && \
		bson_cmp_less_equal_uu (value, max); \

	return bson_cmp_greater_equal_uu (value, 0u) && \
	bson_cmp_less_equal_uu (value, max); \
	return bson_cmp_less_equal_uu (value, max); \

Conversation

eramongodb commented Mar 24, 2022

Uh oh!

rcsanchez97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants